-
-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature: Eventbridge v2: Add input path #10733
Conversation
LocalStack Community integration with Pro 2 files ±0 2 suites ±0 1h 36m 42s ⏱️ +54s Results for commit b9d2dd3. ± Comparison against base commit cbec2f4. This pull request removes 1 and adds 4 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
03803b1
to
76e25d8
Compare
968a98c
to
f05b155
Compare
|
||
def _create_sqs_events_target(queue_name: str | None = None) -> tuple[str, str]: | ||
if not queue_name: | ||
queue_name = f"tests-queue-{short_uid()}" | ||
sqs_client = aws_client.sqs | ||
queue_url = sqs_client.create_queue(QueueName=queue_name)["QueueUrl"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This on purpose does not use the sqs_create_queue
fixture here, since it would need to be moved to helper functions because no cleanup and thus no factory would be required, which in turn would not valid this to be a fixture. This would in turn require passing in the aws_client as input.
I believe it is easier to read if the queue is created with the boot client here directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, fair point 👍
Why is no cleanup required here? Wouldn't the factory remove the need for an explicit cleanup after the yield here? It might have to do with the proper cleanup order 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My main concern is regarding the introduction of an implicit transitive dependency and I'm not quite sure about the typings 😕
Otherwise, it looks good now 👍
|
||
def _create_sqs_events_target(queue_name: str | None = None) -> tuple[str, str]: | ||
if not queue_name: | ||
queue_name = f"tests-queue-{short_uid()}" | ||
sqs_client = aws_client.sqs | ||
queue_url = sqs_client.create_queue(QueueName=queue_name)["QueueUrl"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, fair point 👍
Why is no cleanup required here? Wouldn't the factory remove the need for an explicit cleanup after the yield here? It might have to do with the proper cleanup order 🤔
localstack/services/events/models.py
Outdated
@@ -102,3 +106,20 @@ class InvalidEventPatternException(Exception): | |||
def __init__(self, reason=None, message=None) -> None: | |||
self.reason = reason | |||
self.message = message or f"Event pattern is not valid. Reason: {reason}" | |||
|
|||
|
|||
class FormattedEvent(BaseModel): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how much we gain by defining non-trivial types at this stage. Are we really sure this typing is correct?
It looks quite different than the previous type PutEventsRequestEntry, so at least one of these was/is quite wrong 😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is quite different since the initial defined type was not accurate. We transform the incoming event (here: https://github.com/localstack/localstack/blob/feature/eventbridge_v2-add-input-path/localstack/services/events/provider.py#L129) and add new fields since they are expected by the client.
f05b155
to
31269bf
Compare
31269bf
to
876a1be
Compare
876a1be
to
d0131f3
Compare
d0131f3
to
b9d2dd3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Thanks for removing the implicit pydantic
dependency.
class FormattedEvent(TypedDict): | ||
version: str | ||
id: str | ||
detail_type: Optional[str] # key "detail-type" is automatically interpreted as detail_type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maxhoheiser This type definition allows both detail-type
and detail_type
, but we should restrict it to the hyphen-only variant.
Python supports an alternative syntax for defining TypedDicts, which is recommended in such cases:
The functional syntax should also be used when any of the keys are not valid identifiers, for example because they are keywords or contain hyphens.
https://docs.python.org/3/library/typing.html#typing.TypedDict
All credits go to @dfangl for this suggestion 💯
detail: dict[str, str | dict] | ||
|
||
|
||
TransformedEvent: TypeAlias = FormattedEvent | dict | str |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can understand why we end up such a flexible type definition given the many possibilities using input templates. For readers, this type definition is quite confusing. Consider adding a comment for now.
Maybe, we find something clearer in the future to clarify the type after each step without too many XORs (especially in methods).
Motivation
Enable the input path (transformer) feature (https://docs.aws.amazon.com/eventbridge/latest/APIReference/API_Target.html).
This allows to specify an input path for the target, the body of matching event is parsed based on the specified json path and only the filtered content is sent to the target.
Changes
Add input transformer functionality to rule class.
Add
process_event
method that transforms the event content with the input path transformer if an input path is specified in the target.Change from
send_event
toprocess_event
method invoced by the provider.